Narrow overload-error and symbol-use ranges to terminal identifier#19505
Narrow overload-error and symbol-use ranges to terminal identifier#19505Copilot wants to merge 1 commit into
Conversation
|
@copilot Try again |
Implemented the fix in commit aa4c1a9. The For |
aa4c1a9 to
7d2f1e9
Compare
❗ Release notes required
|
2751d6e to
47b948d
Compare
a5ac31e to
fc504b7
Compare
bfeba1a to
e89bb2d
Compare
Addresses reviewer feedback on PR #19505: - r3058446895 / r3058471629: compute narrow ident range in name resolution so diagnostics and tooling (Find Usages, symbol highlight, FSharpSymbolUse) report on the item name rather than the full long-identifier path. Approach: ComputeItemRange now returns (itemRange, itemIdentRange): - itemRange (wide, unchanged) flows into typed-tree construction, sequence points, and the delay-rest continuation. Structural ranges and the debug experience are untouched. - itemIdentRange (narrow, terminal ident) is consumed only by the CallMethodGroupNameResolutionSink / CallNameResolutionSink calls and RegisterUnionCaseTesterForProperty. This fixes #3920 at the source. The PR #19505 post-hoc trim for FS0041 'No overloads match' is retained because it narrows the overload-resolution diagnostic via column arithmetic and is orthogonal to the sink narrowing. FindReferences test expectations updated for 7 cases to reflect the newly-correct narrow symbol-use ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
09db23f to
18ae276
Compare
Addresses reviewer feedback on PR #19505: - r3058446895 / r3058471629: compute narrow ident range in name resolution so diagnostics and tooling (Find Usages, symbol highlight, FSharpSymbolUse) report on the item name rather than the full long-identifier path. Approach: ComputeItemRange now returns (itemRange, itemIdentRange): - itemRange (wide, unchanged) flows into typed-tree construction, sequence points, and the delay-rest continuation. Structural ranges and the debug experience are untouched. - itemIdentRange (narrow, terminal ident) is consumed only by the CallMethodGroupNameResolutionSink / CallNameResolutionSink calls and RegisterUnionCaseTesterForProperty. This fixes #3920 at the source. The PR #19505 post-hoc trim for FS0041 'No overloads match' is retained because it narrows the overload-resolution diagnostic via column arithmetic and is orthogonal to the sink narrowing. FindReferences test expectations updated for 7 cases to reflect the newly-correct narrow symbol-use ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…14284, #3920) Narrow the FS0041 'No overloads match' error range from the whole expression to just the method name. For T.Instance.Method(""), the error now covers only 'Method' instead of 'T.Instance.Method("")'. Also narrow name-resolution sink ranges (used by Find Usages, symbol highlight, and semantic classification) to the terminal identifier of a dotted long identifier instead of the whole object-expression path. Changes: - NameResolution.fs: ComputeItemRange now returns (itemRange, itemIdentRange). itemRange is the structural whole-long-id span for typed-tree construction. itemIdentRange is the terminal identifier's range for diagnostics and sinks. - CheckExpressions.fs: Intercept UnresolvedOverloading errors in TcMethodApplication and narrow the error range to the method name. - ServiceParamInfoLocations.fs: Update getAllCurriedArgsAtPosition to handle narrowed symbol-use ranges by also checking the leaf function expression. Fixes #14284, #3920 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
5b96681 to
55769f8
Compare
|
|
||
| // mItem is the textual range covered by the long identifiers that make up the item | ||
| and TcItemThen (cenv: cenv) (overallTy: OverallTy) env tpenv (tinstEnclosing, item, mItem, rest, afterResolution) staticTyOpt delayed = | ||
| and TcItemThen (cenv: cenv) (overallTy: OverallTy) env tpenv (tinstEnclosing, item, mItem, _mItemIdent, rest, afterResolution) staticTyOpt delayed = |
There was a problem hiding this comment.
What's the point of adding this parameter (_mItemIdent) that's only discarded?
| /// name. Fixes #14284 and #3920. | ||
| /// | ||
| /// For `T.Instance.Method("")`: | ||
| /// itemRange = `T.Instance.Method` |
There was a problem hiding this comment.
@T-Gro In what cases we'd want to use this range instead of the item one? It seems that when the whole range is required, we'd usually get it from the whole expression. Do some features rely on it?
T-Gro
left a comment
There was a problem hiding this comment.
Review of PR #19505: Narrow overload-error and symbol-use ranges to terminal identifier
Summary
This PR addresses two longstanding issues:
- #14284: FS0041 overload-resolution errors covering the entire expression (e.g.,
T.Instance.Method("")) instead of just the method name - #3920:
FSharpSymbolUseranges reported through the name-resolution sink covering the full dotted long-identifier span instead of the terminal identifier
The approach introduces a dual-range design in ComputeItemRange — separating the structural itemRange (for typed-tree construction) from the narrow itemIdentRange (for diagnostics and IDE symbol reporting). This is well-designed and addresses both issues cleanly.
Strengths
-
Clean dual-range separation:
ComputeItemRangereturning(itemRange, itemIdentRange)is a principled design that keeps expression ranges intact for the typed tree while narrowing only the user-facing diagnostic/IDE surfaces. -
Defensive error-narrowing guards in
TcMethodApplication: The three conditions — single-line check,methodName.Length < itemWidth, andDoesIdentifierNeedBackticks— correctly handle edge cases:- Multiline expressions where column arithmetic would be wrong
- Generic constructors with internal names longer than source text (e.g.,
ImmutableStack1vsImmutableStack) - Backtick-escaped identifiers where
methodName.Lengthdoesn't account for delimiters
-
Thorough test coverage: 7 new regression tests covering static, instance, chained, lambda, backtick, and multiline cases, plus extensive baseline updates across 35+ test files.
-
ServiceParamInfoLocations.fsadaptation: ThegetLeafFuncExprhelper with thenot isInfixguard correctly compensates for narrowed symbol-use positions without breaking infix operator resolution.
Observations / Discussion Points
-
_mItemIdentinTcItemThen(line 8883): As @abonie noted, this parameter is passed through the 6-tuple but immediately discarded. It's carried becauseResolveLongIdentAsExprAndComputeRangereturns it andTcLongIdentThenpasses the full result through, butTcItemThenitself doesn't need it — the narrowing happens either at the sink level (NameResolution.fs) or at the error level (TcMethodApplication). This is correct but adds structural noise. If there's no planned future use withinTcItemThen, consider decomposing the result tuple before the call site (inTcLongIdentThenandTcTypeItemThen) to avoid propagating the unused field. Not a blocker. -
ComputeItemRangewhenrest <> []: When resolution consumes only a prefix (e.g.,System.DateTimefromSystem.DateTime.Now),itemIdentRangefalls back toitemRange(the whole consumed prefix range). Ideally this could narrow to the last consumed identifier (e.g., justDateTime), but since the remaining identifiers are resolved separately viaTcLookupThen→ResolveExprDotLongIdentAndComputeRange, this is benign — the terminalNowgets its own narrow range in the subsequent resolution call. The PR is conservative here and doesn't regress this case. -
getLeafFuncExprperformance: This recursive helper traverses the curried application spine on everyVisitExprcallback. For deeply nested curried calls (e.g.,f a b c d e), this is O(n) per visit. In practice this should be fine since application depth is bounded by typical code, but worth noting. -
@auduchinok's upstream question: The open discussion about whether
mItemshould be computed differently at an earlier stage (rather than trimming atTcMethodApplication) is a valid architectural concern. This PR takes a pragmatic approach: it does the principled narrowing at theComputeItemRangelevel for symbol-use sinks (#3920), while applying a targeted error-range fix atTcMethodApplicationfor overload errors (#14284). Both are useful independently. The deeper refactoring of howmItemflows through the checker could be a follow-up.
Verdict
The changes are correct, well-guarded, and thoroughly tested. The dual-range design is a clean solution that avoids disrupting typed-tree expression ranges while improving diagnostic and IDE precision. The open review comments are non-blocking discussion points rather than correctness issues.
Fixes #14284, #3920
Problem
The FS0041 'No overloads match for method' error covered the entire expression including object access chains and arguments. For
T.Instance.Method(""), the error underlined everything fromTto). This is especially problematic on larger expressions where the wide error range hides other diagnostics.Similarly, FSharpSymbolUse ranges reported through the name-resolution sink (used by Find Usages, symbol highlight, and semantic classification) covered the whole dotted long-identifier span instead of just the terminal identifier.
Changes
NameResolution.fs/fsi
ComputeItemRangenow returns(itemRange, itemIdentRange):ResolveLongIdentAsExprAndComputeRangeandResolveExprDotLongIdentAndComputeRangepropagateitemIdentRangeand use it forCallMethodGroupNameResolutionSinkCheckExpressions.fs
TcMethodApplication: interceptsUnresolvedOverloadingerrors and narrows the range from the whole expression to just the method name, with guards for multiline expressions, generic constructors, and backtick-escaped namesServiceParamInfoLocations.fs
getAllCurriedArgsAtPosition: updated to handle narrowed symbol-use ranges by checking whetherposfalls within the leaf function expression's range (excluding infix applications)Tests
OverloadResolutionErrorRangeTests.fsBefore
Error underlines
T.Instance.Method("")— the entire expression.After
Error underlines only
Method— the method name.